Review: feat: put server, import and lsp behind cargo features

Nice work overall — the split of src/server/ (axum layer) from src/web/ (rendering layer used by both cook server and cook build) is clean, and the bundled_units fix is a genuinely valuable catch (a latent bug where cook recipe's unit output silently depended on feature unification from cooklang-language-server/cooklang-reports rather than a declared dependency). I checked every renamed crate::server::* reference across src/ and templates/ and found no stale paths — the mechanical move looks complete and consistent with the new #[cfg(feature = ...)] gating.

Possible bug / stray test artifact

tests/snapshots/snapshot_test__help_output_no_update.snap.new is checked into this PR (added, not gitignored). A .snap.new file is cargo-insta's "pending, unaccepted" output — its presence means that at some point the actual --help output didn't match the already-accepted snapshot_test__help_output_no_update.snap, and that mismatch was never resolved (accepted or investigated), just committed alongside the .snap it disagrees with.

Concretely: test_help_output is now gated #[cfg(all(feature = "server", feature = "import", feature = "lsp"))], and the _no_update variant additionally requires self-update off — i.e. it only runs under something like --no-default-features --features server,import,lsp. That's exactly the kind of intermediate combination mentioned as manually verified in the PR description, but .github/workflows/test.yml only runs cargo test (all default features) and cargo test --no-default-features — it never builds/tests any of the in-between combos (server alone, server,import,lsp, etc.). So this specific test path isn't covered by CI at all, and the leftover .snap.new is the only evidence that something didn't match during manual testing.

Suggest:

1. Regenerate and diff the two files to see what's actually different, and either update the accepted .snap or delete the stray .snap.new before merging.
2. Add *.snap.new to .gitignore (this repo's CI already uploads them as artifacts on failure at line 78 of test.yml, so they clearly aren't meant to be committed).
3. Consider adding at least one CI job for a partial feature combo (e.g. --no-default-features --features server,import,lsp) so this class of "help output only breaks for one specific feature set" bug can't slip through silently again — this is the same class of bug the bundled_units fix addressed.

Things that look solid on inspection

• sync = ["server", ...] correctly forces server on, and the reasoning (the sync loop only runs inside cook server, so login/logout without a server would store credentials nothing ever consumes) is sound — worth knowing this means "sync without the full HTTP stack" isn't achievable, but that seems like an intentional, reasonable tradeoff rather than an oversight.
• self-update = ["dep:self_update", "dep:reqwest"] explicitly adding dep:reqwest looks redundant at first glance (nothing in update.rs calls reqwest directly), but self_update itself depends on reqwest transitively (confirmed in Cargo.lock), and this repo pins reqwest to default-features = false, features = ["json", "rustls-tls"]. Declaring it explicitly forces Cargo's feature unification to pick rustls instead of whatever TLS backend self_update's own reqwest dependency would default to — this avoids pulling in an OpenSSL-based build, which matters given the PR is specifically fighting low-memory/aarch64 build pain. Good catch, worth calling out in the PR description since it's non-obvious.
• All #[cfg(feature = "server")] additions in src/web/templates.rs (the HTML-page-only templates: ErrorTemplate, ShoppingListTemplate, PreferencesTemplate, PantryTemplate, EditTemplate, NewTemplate, PantrySection/PantryItem) are consistent with what cook build's static-site renderer actually uses (RecipeTemplate/RecipesTemplate/MenuTemplate, which stay ungated) — I didn't find a template referenced by cook build that got accidentally gated behind server.
• Module/command gating in args.rs, lib.rs, main.rs is exhaustive and matches — no leftover unconditional use/mod that would break --no-default-features.

Minor

• README's new feature table is a nice addition; might be worth a one-line callout that sync implies server there too (currently only mentioned in the Cargo.toml comment and commit message), since a reader skimming just the table could reasonably expect sync alone to be lean.

Didn't run a local build in this environment (sandboxed), so I relied on static review of the diff plus grepping for stale references — worth having CI actually confirm the no-default-features/test suite passes clean before merge, per this repo's own pre-PR checklist.

Housekeeping: the bundled_units fix in this PR is now also split out on its own as #371, so it can land independently — it fixes a latent correctness bug that exists on main today and has nothing to do with the refactor here.

That means the two touch the same Cargo.toml line. Merge order:

1. fix: satisfy clippy::useless_borrows_in_formatting (unbreaks CI) #370 — unbreaks CI (clippy is red on main from the Rust 1.97 toolchain bump; that is why this PR shows Test fail despite passing locally)
2. fix(deps): declare cooklang's bundled_units feature explicitly #371 — bundled_units, a verified no-op today
3. this PR — I will rebase onto main afterwards and drop the now-duplicate hunk

This PR still needs bundled_units: without it, --no-default-features drops cooklang-language-server from the graph and recipe output silently changes. That is exactly how the bug was found.

Rebased onto main now that #370, #371 and #368 have landed.

Changes since the last review pass:

• Dropped the duplicate bundled_units hunk (it is on main via fix(deps): declare cooklang's bundled_units feature explicitly #371).
• Removed a stray tests/snapshots/*.snap.new file that a git add -A had swept into the commit, and added *.snap.new to .gitignore so insta rejects cannot slip in again.
• Gated the insta::with_settings import alongside test_help_output, which uses it — clippy 1.97 flagged it as unused under --no-default-features.

Re-verified against the same toolchain CI uses (1.97), not just my local one:

• cargo +1.97 clippy --all-targets -- -D warnings — 0 issues on default, --no-default-features, --all-features, and --features server
• cargo +1.97 fmt --check — clean
• cargo +1.97 test — 13 suites pass on default and --no-default-features

Review

Nice piece of work — the src/server → src/web split (rendering/templates/i18n vs. axum transport) is clean, and moving find_todays_menu's tests along with the function into src/web/menus.rs means no test coverage was actually lost, contrary to what the diff stat suggests at a glance.

Bug: cook server built without lsp breaks the in-browser LSP editor at runtime

Cargo.toml's server feature does not include lsp:

server = ["dep:axum", "dep:askama_axum", "dep:tower", "dep:tower-http", "dep:notify", "dep:notify-debouncer-full", "dep:tokio-stream", "dep:futures-util", "dep:open"]

but src/server/mod.rs unconditionally registers .route("/ws/lsp", get(lsp_bridge::lsp_websocket)), and src/server/lsp_bridge.rs::spawn_lsp_process shells out to the same binary with Command::new(exe_path).arg("lsp").

If someone builds the exact combo the updated README now recommends —

cargo build --release --no-default-features --features server

— the lsp subcommand doesn't exist in that binary. spawn() itself still succeeds (it's spawning a real, existing executable), so spawn_lsp_process's Err branch is never hit; the child process just starts, clap rejects the unrecognized lsp subcommand, and it exits immediately. The websocket bridge then silently breaks (stdin/stdout pipes go dead) rather than failing loudly. This is a runtime issue, not a compile-time one, so the "0 errors/warnings across 9 build combinations" verification in the PR description wouldn't have caught it — and CI (.github/workflows/test.yml) only exercises the "all features" and "no default features" extremes, never --features server alone, so this gap isn't covered there either.

Suggest either:

• adding "lsp" to the server feature list (mirroring how sync already implies server), or
• gating the /ws/lsp route and lsp_bridge module behind #[cfg(feature = "lsp")] so a server-only build simply omits that endpoint instead of registering a route that's guaranteed to fail.

Minor: stray license header on a new, non-vendored file

src/web/mod.rs adds a full MIT license block attributed to Copyright (c) 2024 cooklang. Elsewhere in the repo, that header pattern is reserved for files adapted from Francisco J. Sanchez's original CLI (main.rs, args.rs, shopping_list.rs, server/mod.rs, util/cooklang_to_human.rs, recipe/read.rs, all 2023-attributed, all with the explanatory "the original code is licensed under..." preamble). Genuinely new project files (pantry.rs, search.rs, report.rs, and the other new/renamed files in this PR — web/menus.rs, web/builders.rs, web/templates.rs, sync/mod.rs) carry no such header. This looks like a copy-paste artifact from splitting server/mod.rs rather than an intentional choice — worth double-checking with whoever owns the licensing convention here.

Nit: reqwest under import / self-update features

Worth a second look, lower confidence than the above: neither src/import.rs nor src/update.rs reference reqwest directly (only src/sync/* and src/login.rs do), yet both the import and self-update feature entries in Cargo.toml pull in dep:reqwest. If that's just to force feature unification onto rustls-tls (so cooklang-import's / self_update's own transitive reqwest doesn't drag in a native-TLS/OpenSSL stack alongside it), it's a reasonable and probably intentional pattern — but if not, it's dependency weight the "trim cargo install cookcli" goal of this PR would want to avoid for a standalone --features import or --features self-update build.

Everything else

• The cfg-gating on Command enum variants, mod declarations, and match arms in args.rs / main.rs / lib.rs is consistent and follows the standard "cfg resolves before derive expansion" pattern — should be sound.
• Splitting the always-needed rendering layer (web::builders, web::templates, web::i18n, web::language, StaticFiles) from the axum-specific pieces (kept #[cfg(feature = "server")] inside web::templates.rs / web::language.rs at the item level rather than the module level) correctly keeps cook build working without the server feature.
• sync implying server is explicitly called out and justified in the PR description (the sync loop only runs inside the server); worth flagging as a tradeoff for anyone who'd want a lightweight cook login/cook logout-only build, but not a defect.
• No security concerns — the changes are purely structural/feature-gating; no new attack surface, and CORS/body-limit config is untouched.